-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature table cell stretched #1514
Conversation
@thijskh can you take a look again? I've now done 2 browser hacks to make it easier to change something between either a gecko engine (firefox only at this point?) and something chromium based (Chrome, Edge, Safari & Opera(?)). If this does work for you I can see if I can deduplicate and fix the margin/padding etc. but I prefer to first be sure that this fixes the original issue. |
8dc29b3
to
9d6cae4
Compare
It's not beautiful but seems to do the trick. I don't understand the comments about diffs. |
Ok, than I'll deduplicate the shared parts and squash everything to one commit.
They were placeholders to make sure that the differences between commits would point at the right parts instead of git showing differences between unrelated parts. |
9d6cae4
to
0c2cabd
Compare
I still don't really understand the issue but I assume we get rid of the comments before squashing? |
Yes, |
3375938
to
06d65c1
Compare
Its now 1 commit, and the commit message should explain most (if not all) of the questions. @nickygerritsen / @thijskh I think the browserhack should be stable enough to merge this, can you take a look again? I would prefer to have this in main before the next prelim to test that everything works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works I'm fine with the plan
06d65c1
to
5cc03a6
Compare
ecb9b93
to
e5b3d20
Compare
I think this is now fixed without the browserhack by combining the stuff needed for Firefox & Edge. I've fixed the issue @thijskh found by putting this in a class as most of our tables come from the macro. One annoying thing is the |
See DOMjudge#1273 for the full explaination. This needs some hacks in CSS for firefox & chromium (or the other way around is also possible) given that most major browsers are now chromium based it makes sense to use that as default. The selector `td:not(.testcase-results)` is to make sure that we are reasonable specific to be applied on the tables, it does actually target everything because the testcase-results class doesn't have anchors in the cells. All the anchor tags are now stretched to us the full space inside the TD. Based partly on logic in: https://stackoverflow.com/questions/18488148/how-to-get-div-height-100-inside-td-of-100#comment120502212_18488334 This has as disadvantage that we need the extra classes to keep the rounded buttons (a + border) aligned but as we only have this once it should be accepted.
8ec35a3
to
b9eec15
Compare
This is a partial fix for: #1273, this solution seems to work in firefox but does not work in Edge (chromium based).Given that most major contest do install firefox I think merging this is still acceptable.I've now kept the border to show which element is stretched but that will be removed when merging.I've now implemented a solution with 2 browserhacks. In the end we can set 1 and override the shared properties in the browserhack after it. The browserhack for firefox looks depricated but according to https://sass-lang.com/documentation/breaking-changes/moz-document/ this specific behaviour is kept (so reasonable safe to use).